Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Editor grids permissions refactor, enhancements, and fixes #16653

Open
wants to merge 54 commits into
base: 3.x
Choose a base branch
from

Conversation

smg6511
Copy link
Collaborator

@smg6511 smg6511 commented Nov 29, 2024

Related PR

See also #15919. This is a re-packaging of that PR; this new PR seeks to cover all editor grids across the application and separates out some work (for a future PR) that had been begun around internationalizing core names and descriptions within some of the grids.

Related Issues

#14929, #16507

@smg6511 smg6511 changed the title Grids permissions rework Editor grids permissions refactor, enhancements, and fixes Nov 29, 2024
@smg6511 smg6511 marked this pull request as draft November 29, 2024 04:34
@opengeek
Copy link
Member

opengeek commented Dec 6, 2024

@smg6511 Is this close to being ready?

@smg6511
Copy link
Collaborator Author

smg6511 commented Dec 6, 2024

@opengeek Yes, very close - been hammering away at it. It should be ready for review by early next week.

Formatting, style, optimization only
Changes to js, css, and Lexicon common to multiple areas of this PR
Changes to both grid list and Context editing page (general info)
Implements new permissions handling and fixes a couple other issues in the Dashboard editing panel:
- Added validation to prevent dup Dashboard names
- Styled toggles to match rest of current UI
Formatting, code style changes only
Hides actions icon for first (currently installed) package
Hides actions icon for unchanged Lexicons
Formatting, style updates only
Updates display of and ability to select row actions (gear icon, bulk actions button); includes update to getViewConfig method in base grid class
Formatting, code style changes only
Formatting, code style changes only
Final functional and minor display changes
Formatting, code style changes only
Updates display of and ability to select row actions (gear icon, bulk actions button). Also fixes index controller so users with view permissions can see the grid of Users. Lastly removes unused method for Users grid class.
Formatting, code styling changes only
Formatting, code style changes only
The new getExtrasNamespaces method was needed in places other than the main Namespaces page (via GetList); made static as well. Also updates GetList to use translatable Creator names.
Consolidate shared methods and config elements into new GridBase class
Updates display of and ability to select row actions (gear icon), as well as display of various action buttons. Also adjustments made to base grid class.
Interim base class fixes, updates, additions
Formatting, code style changes only. Also moved menu config from php processor to js class method for consistency.
Formatting, code style changes only
A few new fixes and additions
Apply new permissions methods
Additional cleanup, fine-tuning to application of perms, button creation optimization
Apply new permissions methods
Tweaks, clean up, and application of new create button method to various grids
@smg6511 smg6511 force-pushed the 3.x-grids-permissions-rework branch from 3045679 to 7739175 Compare December 7, 2024 03:12
Copy link

codecov bot commented Dec 7, 2024

Codecov Report

Attention: Patch coverage is 12.29385% with 585 lines in your changes missing coverage. Please review.

Project coverage is 21.38%. Comparing base (b1a02c3) to head (5702f37).
Report is 2 commits behind head on 3.x.

Files with missing lines Patch % Lines
...ion/Processors/System/Dashboard/Widget/GetList.php 0.00% 36 Missing ⚠️
...tion/Processors/Security/Access/Policy/GetList.php 0.00% 33 Missing ⚠️
.../Processors/Workspace/PackageNamespace/GetList.php 0.00% 33 Missing ⚠️
...Revolution/Processors/System/Dashboard/GetList.php 0.00% 28 Missing ⚠️
core/src/Revolution/Sources/modMediaSource.php 0.00% 27 Missing ⚠️
...ssors/Security/Access/UserGroup/Source/GetList.php 0.00% 25 Missing ⚠️
...essors/Security/Access/Policy/Template/GetList.php 0.00% 24 Missing ⚠️
...volution/Processors/System/ContentType/GetList.php 0.00% 24 Missing ⚠️
...ors/Security/Access/UserGroup/Category/GetList.php 0.00% 23 Missing ⚠️
core/src/Revolution/Processors/Source/GetList.php 0.00% 23 Missing ⚠️
... and 27 more
Additional details and impacted files
@@             Coverage Diff              @@
##                3.x   #16653      +/-   ##
============================================
- Coverage     21.53%   21.38%   -0.15%     
- Complexity    10734    10763      +29     
============================================
  Files           565      566       +1     
  Lines         32540    32906     +366     
============================================
+ Hits           7007     7038      +31     
- Misses        25533    25868     +335     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Apply new permissions methods
Formatting, code style changes only
Apply new permissions methods; includes adjustments to base grid class
Formatting, code style changes only
Render links and checkboxes according to user permissions
Remove legacy cls references and mark others for removal
Tweaks and optimizations; fix issue with fields and tvs grids not showing inactive rows properly
@smg6511 smg6511 marked this pull request as ready for review December 11, 2024 04:11
@smg6511 smg6511 added the pr/review-needed Pull request requires review and testing. label Dec 11, 2024
@smg6511
Copy link
Collaborator Author

smg6511 commented Dec 11, 2024

@opengeek @theboxer @Mark-H - Alright, I believe this is finally ready. Hope folks can carve out some time to go through it all ;-) !

@rthrash
Copy link
Member

rthrash commented Dec 12, 2024

@smg6511 this is a big one—kudos on getting it to this stage! Since it's fresh on your mind and touches so many areas, it will take a while to fully review I suspect (was just discussing this with @jaygilmore and @opengeek).

Any suggestions or checklists of all the various nooks and crannies that should be tested to get this one merged?

@smg6511
Copy link
Collaborator Author

smg6511 commented Dec 12, 2024

Any suggestions or checklists of all the various nooks and crannies that should be tested to get this one merged?

Ok, here's generally how I'd suggest going through this:

Initial Runthrough

I overhauled the base grids classes (in modx.grid.js) to remove duplication of existing methods that could be shared and provide access to the new methods introduced from a new central base class; the original Grid, LocalGrid, and JsonGrid extend from this new base. As such, every grid across the app should be interacted with to ensure all works as expected.

Grids with New "Creator" Column and Protected Records

These are the grids where MODX provides built-in records to start with and/or where an Extra installs a record (which usually should only be removed by un-installing the Extra [such as in Namespaces]):

  • Namespaces
  • Roles
  • Contexts
  • Media Sources
  • Dashboards
  • Content Types
  • ACL Policies
  • ACL Policy Templates

A few notable details:

  • Although System Settings type grids would qualify for the same treatment as the above grids, implementation of it is really not feasible at this juncture due to the current data design. The identification of any given record's creator is largely derived or based on some hard-coded value (in the db or in php), as opposed to there being explicit flags in the db (columns where this type of info is persisted).
  • For the most part, I've set most built-in items to be fully protected (i.e., in addition to being non-removable, certain fields are also set immutable [name and sometimes description, or in Content Types the mime type]). What will be carried through separately (in a follow-up PR) is to implement translation on these immutables fields.
  • Following from the above point, matching field behavior has been implemented for a protected record's editing page (i.e., any field that was immutable on the grid is also immutable in the editing page [e.g., see Media Sources and Contexts]). Additionally, a new note is shown at the top of the panel alerting users re the item being protected and that some built-in fields may not be changed.

All Editor Grids

  • Buttons and button menu items are either hidden or set to disabled when permissions for their action is not granted.
  • Bulk Actions button menu items should, additionally, only be active when selections have been made (this is regardless of permissions on their respective actions).
  • Some grids depend on the permissions of related objects when rendering certain items. A prime example is the Template Variables grid in a Template's editing panel: it renders a link on each TV name leading to that TV's editing page (but now, only if permission to edit & save TVs is granted). The various Access Permissions grids also cross link in this way. Pay attention to these direct-linked items as you review each area.
  • Pencil icon should only show for editable fields (based on permissions)
  • Gear menu should only show when there is at least one action on a row that the user has permission to take; otherwise it is hidden

General Strategy

As I was working through this PR, I kept one browser open with an Admin user with full permissions and another with an alternate user with permissions I'd vary to check my work. I basically systematically went through each nav menu item and completed updates to all relevant grids for one area at a time. I'd suggest the same for reviewing. Remember there are some grids that are a couple hops away, and can be easy to miss:

  • TVs: There are multiple grids within this editing page to review (primarily the links to related objects [Contexts, Templates, User Groups] is what you'll want to pay attention to)
  • Trash Manager
  • User Group > Access Permissions (5 grids with various cross-linked objects)

@opengeek opengeek modified the milestones: v3.1.0, v3.1.1 Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/review-needed Pull request requires review and testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants